enhance navbar#617
Conversation
- Added animated sliding pill for desktop navigation hover states - Refactored nav links into a reusable navItems array - Added scroll detection for dynamic navbar glass effect - Applied backdrop blur and translucent background on scroll - Improved navbar visual polish in both light and dark modes - Fixed mobile theme toggle icon visibility - Added custom opening animation for mobile menu
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughNavbar component refactored with a centralized ChangesNavbar modernization with scroll tracking, sliding pill, and mobile menu improvements
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/index.css (2)
94-104: ⚡ Quick winPrefer transform/opacity-only animation to avoid expensive blur repainting.
Animating
filter: blur()here can introduce visible jank on low-end/mobile devices. Keep the effect toopacity + transformfor smoother menu open.Proposed change
`@layer` utilities{ `@keyframes` fade-in{ from { opacity:0; transform: translateY(20px); - filter:blur(45px); } to{ opacity:1; transform: translateY(0); - filter:blur(0); } } .ani-fade-in{ animation: fade-in 0.8s ease-out both; } }Also applies to: 106-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.css` around lines 94 - 104, The keyframes animation "fade-in" (and the similar block at lines 106-108) currently animates filter: blur(), which is expensive; remove the filter property from both from/to blocks and only animate opacity and transform (translateY) so the menu opens smoothly on low-end devices — update the `@keyframes` fade-in and the other keyframe to drop filter: blur(…) entries and ensure only opacity and transform are animated.
93-109: ⚡ Quick winAdd reduced-motion fallback for the mobile menu animation.
There’s no
prefers-reduced-motionhandling, so motion-sensitive users still get the transition. Add an opt-out utility override.Proposed change
`@layer` utilities{ `@keyframes` fade-in{ @@ .ani-fade-in{ animation: fade-in 0.8s ease-out both; } + + `@media` (prefers-reduced-motion: reduce) { + .ani-fade-in { + animation: none; + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.css` around lines 93 - 109, Add a prefers-reduced-motion fallback so users who opt out of motion aren't animated: wrap an `@media` (prefers-reduced-motion: reduce) rule that targets the fade-in keyframes and the .ani-fade-in utility (the `@keyframes` fade-in and the .ani-fade-in class) and override the animation to none (and set static properties like opacity:1 and transform:none if needed) so the mobile menu appears instantly for reduced-motion users.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Navbar.tsx`:
- Around line 22-29: The useEffect adds a scroll listener but never initializes
scrolled, so call the handler once on mount and add the listener as passive;
specifically, inside the useEffect where handleScroll(), setScrolled, and
window.addEventListener("scroll", handleScroll) are used, invoke handleScroll()
immediately after defining it and change the addEventListener call to use {
passive: true } so the navbar state is correct on initial render and the scroll
listener is passive.
---
Nitpick comments:
In `@src/index.css`:
- Around line 94-104: The keyframes animation "fade-in" (and the similar block
at lines 106-108) currently animates filter: blur(), which is expensive; remove
the filter property from both from/to blocks and only animate opacity and
transform (translateY) so the menu opens smoothly on low-end devices — update
the `@keyframes` fade-in and the other keyframe to drop filter: blur(…) entries
and ensure only opacity and transform are animated.
- Around line 93-109: Add a prefers-reduced-motion fallback so users who opt out
of motion aren't animated: wrap an `@media` (prefers-reduced-motion: reduce) rule
that targets the fade-in keyframes and the .ani-fade-in utility (the `@keyframes`
fade-in and the .ani-fade-in class) and override the animation to none (and set
static properties like opacity:1 and transform:none if needed) so the mobile
menu appears instantly for reduced-motion users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e65b63c3-d933-4ca8-ab1a-6fd01978b39f
📒 Files selected for processing (2)
src/components/Navbar.tsxsrc/index.css
| useEffect( () => { | ||
| const handleScroll = () => { | ||
| setScrolled(window.scrollY>20); | ||
| }; | ||
|
|
||
| window.addEventListener("scroll",handleScroll); | ||
| return () => window.removeEventListener("scroll",handleScroll); | ||
| },[]); |
There was a problem hiding this comment.
Initialize scroll-derived state on mount.
scrolled is only set after the first scroll event, so loading a page at a non-zero scroll position renders the wrong navbar style initially. Call the handler once in the effect (and make the listener passive).
Suggested fix
useEffect( () => {
const handleScroll = () => {
setScrolled(window.scrollY>20);
};
- window.addEventListener("scroll",handleScroll);
- return () => window.removeEventListener("scroll",handleScroll);
+ handleScroll();
+ window.addEventListener("scroll", handleScroll, { passive: true });
+ return () => window.removeEventListener("scroll", handleScroll);
},[]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect( () => { | |
| const handleScroll = () => { | |
| setScrolled(window.scrollY>20); | |
| }; | |
| window.addEventListener("scroll",handleScroll); | |
| return () => window.removeEventListener("scroll",handleScroll); | |
| },[]); | |
| useEffect( () => { | |
| const handleScroll = () => { | |
| setScrolled(window.scrollY>20); | |
| }; | |
| handleScroll(); | |
| window.addEventListener("scroll", handleScroll, { passive: true }); | |
| return () => window.removeEventListener("scroll", handleScroll); | |
| },[]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Navbar.tsx` around lines 22 - 29, The useEffect adds a scroll
listener but never initializes scrolled, so call the handler once on mount and
add the listener as passive; specifically, inside the useEffect where
handleScroll(), setScrolled, and window.addEventListener("scroll", handleScroll)
are used, invoke handleScroll() immediately after defining it and change the
addEventListener call to use { passive: true } so the navbar state is correct on
initial render and the scroll listener is passive.
|
@itsdakshjain please review this PR |
itsdakshjain
left a comment
itsdakshjain
left a comment
There was a problem hiding this comment.
hi @Luffy-456 thanks for the contribution, new effect on the navbar looks good
as you can see from the bot comment below, there is just one tiny bug we need to fix in Navbar.tsx before merging. right now, the scroll styling only applies in when a user actively moves the screen. if someone reloads the page while they are already sitting halfway down, the navbar won't style correctly until they start scrolling again.
let's fix that by running the handleScroll() function once right when the component mounts:
useEffect(() => {
const handleScroll = () => {
setScrolled(window.scrollY > 20);
};
handleScroll(); // checks scroll position immediately on load
window.addEventListener("scroll", handleScroll, { passive: true });
return () => window.removeEventListener("scroll", handleScroll);
}, []);note for maintainers: the assigned labels are correct as per gssoc guidelines. also, please add the mentor label mentor:itsdakshjain to this pr.
@mehul-m-prajapati
once that quick change is pushed
ready to merge
2 participants
Related Issue
Description
The current navbar is functional but lacks a modern and visually appealing design. The overall appearance feels basic and doesn't fully align with UI/UX standards used in modern web applications.
Changes Made:
navItemsarrayHow Has This Been Tested?
Screenshots (if applicable)
2026-05-30.13-30-07.mp4
Type of Change